Skip to content

fix: add setters to CJS compatibility layer in Settings#7481

Merged
JohnMcLear merged 1 commit intodevelopfrom
fix/settings-cjs-setters
Apr 6, 2026
Merged

fix: add setters to CJS compatibility layer in Settings#7481
JohnMcLear merged 1 commit intodevelopfrom
fix/settings-cjs-setters

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

Summary

  • The CJS compatibility block added in fd97532 only defined getters on module.exports, making settings read-only for CJS consumers.
  • Plugins like ep_webrtc need to mutate settings in tests (e.g. settings.requireAuthentication = false), which throws TypeError: Cannot set property which has only a getter.
  • Add set to the property descriptors so CJS consumers can both read and write settings.

Test plan

  • Core backend tests still pass
  • ep_webrtc CI passes after this lands

🤖 Generated with Claude Code

The CJS compatibility block added in fd97532 only defined getters,
making settings properties read-only for plugins using require().
Plugins like ep_webrtc need to mutate settings (e.g. requireAuthentication)
in tests. Add setters so CJS consumers can write properties too.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Add setters to CJS compatibility layer in Settings

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Add setters to CJS compatibility layer in Settings module
• Enables CJS consumers to mutate settings properties
• Fixes TypeError when plugins write to settings
• Allows ep_webrtc and similar plugins to modify settings in tests
Diagram
flowchart LR
  A["CJS Consumer"] -->|"read/write settings"| B["Settings Module"]
  B -->|"defineProperty with getter+setter"| C["module.exports"]
  C -->|"access settings"| D["Internal settings object"]
Loading

Grey Divider

File Changes

1. src/node/utils/Settings.ts 🐞 Bug fix +1/-0

Add setter to CJS property descriptors

• Added setter function to property descriptor in CJS compatibility block
• Setter allows writing to settings properties via (settings as any)[key] = v
• Maintains existing getter, enumerable, and configurable properties
• Enables plugins to mutate settings in tests without TypeError

src/node/utils/Settings.ts


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects bot commented Apr 6, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. No regression test for setter 📘 Rule violation ☼ Reliability
Description
This bug fix changes module.exports property descriptors to add a setter, but the PR does not
include any accompanying regression test to ensure CJS consumers can mutate settings. Without an
automated test, the issue can silently reoccur if this change is reverted or refactored.
Code

src/node/utils/Settings.ts[R670-676]

    if (!(key in currentExports)) {
      Object.defineProperty(currentExports, key, {
        get: () => (settings as any)[key],
+        set: (v: any) => { (settings as any)[key] = v; },
        enumerable: true,
        configurable: true,
      });
Evidence
PR Compliance ID 1 requires a regression test for bug fixes. The only change shown is the setter
addition in Settings.ts, and no test additions/updates are included alongside this fix.

src/node/utils/Settings.ts[670-676]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A bug fix was made to allow CJS consumers to mutate settings (by adding a `set` handler in the `module.exports` compatibility block), but no regression test was added.

## Issue Context
Without a test, a future refactor (or revert) could reintroduce `TypeError: Cannot set property which has only a getter` for CJS consumers.

## Fix Focus Areas
- src/node/utils/Settings.ts[670-676]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. CJS misses new settings keys 🐞 Bug ≡ Correctness
Description
The CJS compatibility layer only defines getters/setters for Object.keys(settings) at module
initialization, so keys added later by storeSettings() (including plugin-specific ep_* keys) are
not accessible or writable via require('.../Settings').<key> even though they exist on the
underlying settings object.
Code

src/node/utils/Settings.ts[R670-676]

    if (!(key in currentExports)) {
      Object.defineProperty(currentExports, key, {
        get: () => (settings as any)[key],
+        set: (v: any) => { (settings as any)[key] = v; },
        enumerable: true,
        configurable: true,
      });
Evidence
The CJS shim defines properties only for the initial default keys (before configuration is loaded).
Later, reloadSettings() calls storeSettings(), which explicitly supports plugin-specific keys
(ep_*) and assigns them onto the settings object—after the shim has already run—so those new
keys will not be proxied onto module.exports.

src/node/utils/Settings.ts[664-679]
src/node/utils/Settings.ts[731-753]
src/node/utils/Settings.ts[959-963]
src/node/utils/Settings.ts[1120-1121]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The CommonJS compatibility shim only defines properties for the keys present in `settings` at module initialization. Keys added later (for example `ep_*` plugin settings populated by `storeSettings()` during `reloadSettings()`) are not exposed on `module.exports`, so CJS consumers cannot read/write them via `require(.../Settings).ep_plugin`.

### Issue Context
- The shim runs immediately after `export default settings`.
- `reloadSettings()` later calls `storeSettings()` which can add new keys (notably `ep_*`).

### Fix Focus Areas
- src/node/utils/Settings.ts[664-679]
- src/node/utils/Settings.ts[731-759]
- src/node/utils/Settings.ts[959-964]

### Suggested fix
Add a small helper (e.g., `ensureCjsExportKey(key: string)`) that defines the same getter/setter on `module.exports` for a given key if it is missing. Call it:
1) Inside the existing initialization loop (current behavior), and
2) Inside `storeSettings()` after assigning `settings[i]` (especially when `i.startsWith('ep_')` or when `settings[i]` transitions from `undefined` to defined).

This keeps the CJS surface in sync with dynamically-added settings keys.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines 670 to 676
if (!(key in currentExports)) {
Object.defineProperty(currentExports, key, {
get: () => (settings as any)[key],
set: (v: any) => { (settings as any)[key] = v; },
enumerable: true,
configurable: true,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. No regression test for setter 📘 Rule violation ☼ Reliability

This bug fix changes module.exports property descriptors to add a setter, but the PR does not
include any accompanying regression test to ensure CJS consumers can mutate settings. Without an
automated test, the issue can silently reoccur if this change is reverted or refactored.
Agent Prompt
## Issue description
A bug fix was made to allow CJS consumers to mutate settings (by adding a `set` handler in the `module.exports` compatibility block), but no regression test was added.

## Issue Context
Without a test, a future refactor (or revert) could reintroduce `TypeError: Cannot set property which has only a getter` for CJS consumers.

## Fix Focus Areas
- src/node/utils/Settings.ts[670-676]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the latest push. Thanks for catching this!

@JohnMcLear JohnMcLear merged commit 8c1b8b0 into develop Apr 6, 2026
36 checks passed
@JohnMcLear JohnMcLear deleted the fix/settings-cjs-setters branch April 6, 2026 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant